feat(support): unicode-aware glob matching with byte fallback - #179
feat(support): unicode-aware glob matching with byte fallback#17916bit-ykiko wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughGlob matching now decodes valid UTF-8 into Unicode code points for wildcards, brackets, and escaped literals. Brackets store Unicode ranges with negation support, while invalid UTF-8 bytes remain single-byte literals. Tests cover Unicode, malformed input, wildcard, and range behavior. ChangesUnicode glob matching
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Pattern as GlobPattern::SubGlobPattern::create
participant Decoder as UTF-8 atom decoder
participant Matcher as glob match engine
Pattern->>Decoder: Decode bracket and escaped pattern atoms
Decoder-->>Pattern: Valid code points or literal invalid bytes
Pattern->>Matcher: Store Unicode ranges and negation
Matcher->>Decoder: Decode input atom
Decoder-->>Matcher: Code point and byte length
Matcher->>Matcher: Apply wildcard, bracket, or escaped-literal match
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/support/glob_pattern.cpp (1)
370-374: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: inconsistent error-propagation style.
The rest of
create()/its lambdas useKOTA_EXPECTED_TRY_V(e.g. line 104, 314, 317) forstd::expectedpropagation, but this spot manually checkshas_value()/error(). Purely stylistic.♻️ Optional consistency tweak
- auto ranges = parse_bracket_charset(invert ? chars.substr(1) : chars); - if(!ranges.has_value()) [[unlikely]] { - return std::unexpected{std::move(ranges.error())}; - } - pat.brackets.push_back(Bracket{j + 1, invert, std::move(*ranges)}); + KOTA_EXPECTED_TRY_V(auto ranges, parse_bracket_charset(invert ? chars.substr(1) : chars)); + pat.brackets.push_back(Bracket{j + 1, invert, std::move(ranges)});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/support/glob_pattern.cpp` around lines 370 - 374, In the bracket parsing flow of create(), replace the manual ranges.has_value() check and std::unexpected error return with the existing KOTA_EXPECTED_TRY_V propagation style, while preserving the resulting ranges value for the subsequent pat.brackets.push_back call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/support/glob_pattern.cpp`:
- Around line 370-374: In the bracket parsing flow of create(), replace the
manual ranges.has_value() check and std::unexpected error return with the
existing KOTA_EXPECTED_TRY_V propagation style, while preserving the resulting
ranges value for the subsequent pat.brackets.push_back call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ef67048-0ca9-4682-971c-338cea36ae1a
📒 Files selected for processing (3)
include/kota/support/glob_pattern.hsrc/support/glob_pattern.cpptests/unit/support/glob_pattern_tests.cpp
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
kotatsu/src/support/glob_pattern.cpp
Line 664 in 1f380af
Advance wildcard backtracking by UTF-8 atoms
When * backtracks over a valid multibyte character, incrementing state.s by one byte lets the next matcher start on a continuation byte. That byte is then decoded as a standalone invalid atom, violating the documented rule that invalid bytes only equal themselves; for example, a pattern consisting of "*[" + byte(0xA9) + "]" (or an escaped/literal 0xA9) incorrectly matches the valid UTF-8 subject "é" (C3 A9) after * consumes only C3. Advance to the next decoded atom boundary instead of testing positions inside valid UTF-8 sequences.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const bool hit = std::ranges::any_of(ranges, [&](const auto& range) { | ||
| return range.first <= cp && cp <= range.second; | ||
| }); |
There was a problem hiding this comment.
For a pattern containing * followed by a large character class, every wildcard backtrack calls this linear scan, so the existing 65,536-iteration ReDoS cap no longer bounds matching work as it did with the constant-time bitset lookup. For example, a class containing 100,000 repeated nonmatching members and a 65,536-byte subject can trigger billions of range comparisons; merge duplicate/overlapping ranges or otherwise cap/index them so crafted patterns cannot multiply the backtracking limit by the class length.
Useful? React with 👍 / 👎.
| static_cast<std::uint32_t>(hi))} | ||
| }; | ||
| } | ||
| ranges.push_back({*pending, hi}); |
There was a problem hiding this comment.
When one endpoint is an invalid UTF-8 byte, its synthetic value above 0x10FFFF participates in ordinary range ordering, so a malformed pattern such as [a-\x80] unexpectedly matches every valid code point from a through U+10FFFF (including 中). This contradicts the new byte-fallback contract that an invalid byte only compares equal to itself; reject ranges mixing synthetic and Unicode atoms or give malformed endpoints byte-only range semantics.
Useful? React with 👍 / 👎.
1 participant
Problem
GlobPatternmatched byte-by-byte with abitset<256>character class, as its own doc comment admitted: multi-byte UTF-8 characters were never matched by?(one byte each) and[...]ranges over non-ASCII were meaningless. VS Code's own glob engine matches?against a whole character, so patterns over CJK paths behaved differently from the syntax's reference implementation.Approach
The matcher strategy follows rust-lang/glob (matching over decoded code points) combined with glibc
fnmatch's precedent of falling back to bytes for invalid sequences:?,[...]and\-escaped literals now consume one decoded UTF-8 code point at a time.[lo, hi]code-point ranges instead of a byte bitset; negation is evaluated at match time (a bracket still never matches/).?consumes exactly one such byte, and nothing crashes or mis-aligns.*,**, literal comparison and backtracking stay byte-level on purpose: UTF-8 is self-synchronizing and/is ASCII, so byte stepping cannot change match semantics there.Brace expansion, segment handling, prefix extraction and the ReDoS backtrack cap are untouched.
Testing
unicode_question,unicode_bracket,unicode_escape_star,invalid_utf8_bytes, plusported_wildcards/ported_rangesadapted from rust-lang/glob's test suite (MIT/Apache-2.0).Summary by CodeRabbit
New Features
Bug Fixes
Tests